Generalize mode-overlap detectors and S-parameter calculation to custom reference modes - #405
Conversation
📝 WalkthroughWalkthroughMode-overlap detection is refactored around a shared base class, with custom and analytic Gaussian reference modes, tilted polarization handling, generalized S-parameter support, public exports, documentation, and unit/physics validation. ChangesMode overlap detector extensions
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Simulation
participant GaussianPlaneSource
participant GaussianModeOverlapDetector
participant ComputeOverlap
Simulation->>GaussianPlaneSource: generate Gaussian beam fields
Simulation->>GaussianModeOverlapDetector: apply detector-plane data
GaussianModeOverlapDetector-->>Simulation: store analytic reference modes
Simulation->>ComputeOverlap: compute modal overlap
ComputeOverlap-->>Simulation: return overlap power
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
==========================================
+ Coverage 90.35% 90.52% +0.17%
==========================================
Files 92 92
Lines 11789 11920 +131
Branches 1803 1820 +17
==========================================
+ Hits 10652 10791 +139
+ Misses 794 783 -11
- Partials 343 346 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @bruxillensis, thanks for another PR! I skimmed over the code and the structural changes, which all look/sound good to me. If you want to mark the PR as ready for review I am happy to give a more detailed look at the code. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fdtdx/objects/detectors/mode.py`:
- Around line 105-115: Update the Gaussian envelope calculation in the tilted
branch of the detector reference-field implementation to use the coordinate
displacement projected orthogonally to wave_vector, matching
GaussianPlaneSource’s beam-plane footprint; retain the existing detector-plane
calculation for untilted waves and add a regression comparing the tilted
detector profile against the source projection.
- Around line 148-164: Update both analytic Gaussian-mode paths, including the
flow around gaussian_mode_fields, to retain and propagate inv_permeability_slice
through the helper/factory instead of discarding it. Compute wavenumber using
the effective permittivity and permeability product, and scale the
magnetic/electric field relationship using their ratio. Add coverage for a
non-unit-permeability medium and verify the generated mode and overlap
normalization.
- Around line 25-40: Validate radius at the start of gaussian_mode_fields and
reject zero or negative values before computing the Gaussian exponent,
normalization, or overlaps. Raise the established argument-validation exception
with a clear message, while preserving behavior for positive radii.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c14361c5-c9fa-48df-858e-ca3ceba7ebf0
📒 Files selected for processing (7)
docs/source/07_api.rstsrc/fdtdx/__init__.pysrc/fdtdx/core/misc.pysrc/fdtdx/objects/detectors/mode.pysrc/fdtdx/objects/sources/linear_polarization.pysrc/fdtdx/utils/sparams.pytests/unit/objects/detectors/test_mode_custom.py
|
Marked! Its exciting to see the contributor growth, you have many PR's to review now! |
|
|
||
| @autoinit | ||
| class GaussianModeOverlapDetector(BaseModeOverlapDetector): | ||
| """Mode-overlap detector using an analytic Gaussian beam as the reference mode. |
There was a problem hiding this comment.
I would maybe say "Tilted Gaussian" here not "Gaussian Beam" to avoid confusion with focused Gaussian beam, or generally disambiguate this difference a bit in the docstrings
|
I gave the code a closer look, one more comment in addition to the ones above:
Other than that, the code looks good to me! |
d330666 to
823e356
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/objects/detectors/test_mode_custom.py (2)
356-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
_plane_coords()helper.
TestGaussianModeGeneralization._plane_coords(356-360) andTestGaussianRadiusValidation._plane_coords(463-467) are byte-for-byte identical. Consider hoisting to a module-level function/fixture to avoid the two copies drifting apart later.Also applies to: 463-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/objects/detectors/test_mode_custom.py` around lines 356 - 360, Remove the duplicate _plane_coords methods from TestGaussianModeGeneralization and TestGaussianRadiusValidation, define one shared module-level helper or fixture with the same meshgrid behavior, and update both test classes to use it.
236-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring on
mk().
mk's docstring says "Build a placed detector for the given mode function," but it actually broadcasts a per-pole ADE coefficient scalar into a(1, 1, 8, 8, 8)array — a leftover/copy-paste artifact that misdescribes the helper.📝 Proposed fix
def mk(a): # broadcast a per-pole scalar to (num_poles, 1, Nx, Ny, Nz) - """Build a placed detector for the given mode function.""" + """Broadcast a per-pole coefficient to the dispersion-array shape.""" return jnp.full((1, 1, 8, 8, 8), float(a[0]), dtype=jnp.float32)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/objects/detectors/test_mode_custom.py` around lines 236 - 245, Update the nested helper mk inside _dispersive_coeffs so its docstring describes broadcasting a per-pole ADE coefficient scalar into the detector-shaped (1, 1, 8, 8, 8) array; remove the stale reference to building a placed detector from a mode function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/objects/detectors/test_mode_custom.py`:
- Around line 356-360: Remove the duplicate _plane_coords methods from
TestGaussianModeGeneralization and TestGaussianRadiusValidation, define one
shared module-level helper or fixture with the same meshgrid behavior, and
update both test classes to use it.
- Around line 236-245: Update the nested helper mk inside _dispersive_coeffs so
its docstring describes broadcasting a per-pole ADE coefficient scalar into the
detector-shaped (1, 1, 8, 8, 8) array; remove the stale reference to building a
placed detector from a mode function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0acd3ca5-2ec2-4c1f-85f8-a3d0090d8ac6
📒 Files selected for processing (9)
docs/source/07_api.rstsrc/fdtdx/__init__.pysrc/fdtdx/core/misc.pysrc/fdtdx/objects/detectors/mode.pysrc/fdtdx/objects/sources/linear_polarization.pysrc/fdtdx/utils/sparams.pytests/simulation/physics/detectors/__init__.pytests/simulation/physics/detectors/test_mode_overlap.pytests/unit/objects/detectors/test_mode_custom.py
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/source/07_api.rst
- src/fdtdx/init.py
- src/fdtdx/utils/sparams.py
- src/fdtdx/objects/sources/linear_polarization.py
- src/fdtdx/core/misc.py
- src/fdtdx/objects/detectors/mode.py
|
@ymahlau I think this is ready for another look. Was on a work trip for the past few weeks. Comments were addressed, and I added divergence_angle since the original implementation assumed a perfectly collimated beam. It feels slightly unintuitive to me still to construct a gaussian detector (as in, you need to understand how the parameters are used), but maybe thats unavoidable. Open to feedback on it. |
|
I also can't think of a better solution from the top of my head, but I think it is also okay to expect a little understanding of what the parameters do. Thanks for the PR! |
2 participants
Summary
ModeOverlapDetector currently only supports reference modes produced by the tidy3d waveguide mode solver. My motivation was that I wanted to use ModeOverlapDetector for gaussians and custom (user defined or solved for) modes, for example with inverse design of a grating into a radiating mode. This PR splits the overlap machinery into an abstract base class and adds two new detectors that overlap the simulated fields against user-supplied or analytic reference modes — enabling coupling-efficiency and S-parameter workflows for free-space beams or modes imported from other tools, without involving the mode solver.
What changed
Usage
No behavior change for existing ModeOverlapDetector users; the refactor is purely structural and all public APIs are preserved.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests